Skip to content

fix: modify change password API v3 endpoints - #655

Open
alemar99 wants to merge 2 commits into
canonical:3.8from
alemar99:backport-ffcb72f-3.8
Open

alemar99 wants to merge 2 commits into
canonical:3.8from
alemar99:backport-ffcb72f-3.8

Conversation

@alemar99

@alemar99 alemar99 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

(cherry picked from commit ffcb72f)
(cherry picked from commit a72c0ea)

@alemar99 alemar99 added the 3.8 label Sep 14, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maas-code-reviewer review]

LLM-generated review from https://github.com/canonical/maas-code-reviewer.
Intended to assist a human reviewer, not replace one — suggestions may be
incorrect, please verify before acting.

This PR successfully splits the user update requests into Self and Admin variants and correctly moves password validation into a separate check before updating. However, there are two important issues to address:

  1. Unintentional Overwrite of is_staff/is_active: Hardcoding these fields to False and True in the to_builder() methods causes any profile update to overwrite them. This means staff users will accidentally demote themselves when updating their names.
  2. Event Loop Blocking: Password hashing and verification are CPU-bound operations. Running them synchronously blocks the asyncio event loop and introduces a Denial-of-Service vulnerability.

Comment thread src/maasapiserver/v3/api/public/models/requests/users.py
Comment thread src/maasapiserver/v3/api/public/models/requests/users.py
Comment thread src/maasservicelayer/services/users.py Outdated
Comment thread src/tests/maasapiserver/v3/api/public/handlers/test_users.py
@AloizioMacedo

Copy link
Copy Markdown
Contributor

(For future reference): This is, in part, a followup to #623.

AloizioMacedo
AloizioMacedo previously approved these changes Sep 15, 2026
Wrap the `PBKDF2PasswordHasher` method calls in `asyncio.to_thread` to
avoid blocking the event loop.

(cherry picked from commit a72c0ea)
@alemar99

Copy link
Copy Markdown
Contributor Author

@AloizioMacedo I've also included here the commit for running password hashing in a thread. Let me know if that works for you or if we want to have them split.

@AloizioMacedo

Copy link
Copy Markdown
Contributor

@AloizioMacedo I've also included here the commit for running password hashing in a thread. Let me know if that works for you or if we want to have them split.

It works, but let's wait on #662.

Theoretically, it seems there are not many conflicts (if any), but both touch things related to password (FIPS adds some validation).

@alemar99 alemar99 added the blocked do not merge this label Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.8 blocked do not merge this

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants